[FIX] Incorrect display of "Reply in Direct Message" in MessageAction #17968
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Check that we either already have a DM room started with the message user or that we have the `create-d` permission to start one, otherwise hiding the reply in DM menu option. Currently if a user does not have the `create-d` permission and does not already have a DM started with a user, the "Reply in Direct Message" context menu is still displayed for messages even though clicking it will show the user a "User not found - Invalid room" page and "Not allowed [error-not-allowed]" toast message.
MartinSchoeler
requested changes
Jul 29, 2020
|
|
||
| // Check if we already have a DM started with the message user (not ourselves) or we can start one | ||
| const dmRoom = Rooms.findOne({ _id: [u._id, msg.u._id].sort().join('') }); | ||
| const canAccessDM = dmRoom && Subscriptions.findOne({ rid: dmRoom._id, 'u._id': u._id }); |
There was a problem hiding this comment.
Maybe before doing multiple findOnes, we can check if the user lack the permission, so we can avoid calling these methods when we don't need it.
There was a problem hiding this comment.
Are you suggesting something like this?
// Don't show reply option if we're messaging someone else and we don't have
// permission to start a new DM with them
if (u._id !== msg.u._id && !hasPermission('create-d')) {
const dmRoom = Rooms.findOne({ _id: [u._id, msg.u._id].sort().join('') });
if (!dmRoom || !Subscriptions.findOne({ rid: dmRoom._id, 'u._id': u._id })) {
return false;
}
}
There was a problem hiding this comment.
@MartinSchoeler are you suggesting something like ☝️ ?
There was a problem hiding this comment.
@MartinSchoeler thoughts RE my suggested fix?
There was a problem hiding this comment.
hi @MartinSchoeler still awaiting your feedback.
MartinSchoeler
approved these changes
Jan 20, 2021
|
What's needed to get this moving along @MartinSchoeler ? |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
Check that we either already have a DM room started with the message user or that we have the
create-dpermission to start one, otherwise hiding the reply in DM menu option.Issue(s)
Currently if a user does not have the
create-dpermission and does not already have a DM started with a user, the "Reply in Direct Message" context menu option is still displayed for messages even though clicking it will show the user a "User not found - Invalid room" page and "Not allowed [error-not-allowed]" toast message.Screenshots
Types of changes
Checklist
Changelog
[FIX] Incorrect display of "Reply in Direct Message" in MessageAction
Further comments
Note I've left the tests in a separate commit for now as they weren't super simple due to some of the setup required to test this. I tried to find other examples of tests that did similar things but didn't really find anything suitable. Would be good to get some feedback as to if there would be a better way to do the setup?